chore: partner-offer - standardise conversation offer CTA - #13736
chore: partner-offer - standardise conversation offer CTA#13736Mitch-Henson wants to merge 1 commit into
Conversation
oxaudo
left a comment
oxaudo
left a comment
There was a problem hiding this comment.
Makes sense to me. Couple of very minor comments.
I still think that eventually would make sense to test if direct purchase buttons might convert better here but def support UI being the same regardless if it's personalized or bulk offer.
| const isOfferFromSaves = item?.partnerOffer?.source === "SAVE" | ||
|
|
||
| let subtitle = isOfferFromSaves | ||
| let subtitle: string | null = isOfferFromSaves |
There was a problem hiding this comment.
think in cases like this && reads better. Something like subtitle = isOfferFromSaves && xxx
| expect(screen.getByText(/List price:\s*\$450,000\s*/)).toBeOnTheScreen() | ||
| expect(screen.getByText('"This is a note from the gallery"')).toBeOnTheScreen() | ||
| expect(screen.getByText("Review the offer before it expires")).toBeOnTheScreen() | ||
| expect(screen.queryByText("Review the offer before it expires")).not.toBeOnTheScreen() |
There was a problem hiding this comment.
With this changes this .not test becomes more confusing than anything.... Like we could have texted for any random text not there? One would not be able to find it in code as now it would never appear. I would just remove this line.
| <Spacer y={0.5} /> | ||
|
|
||
| <Text variant="sm-display">{subtitle}</Text> | ||
| {!!subtitle && <Text variant="sm-display">{subtitle}</Text>} |
There was a problem hiding this comment.
In fact as it now only one possible case I would just inline it here. Subtitle is always the same now so we could text !!isOfferFromSaves && <Text>...</Text>
3 participants
This PR resolves this notion card
Related Force PR: artsy/force#17443
Description
PR Checklist
To the reviewers 👀
Changelog updates
Changelog updates
Cross-platform user-facing changes
iOS user-facing changes
Android user-facing changes
Dev changes
Need help with something? Have a look at our docs, or get in touch with us.
cc: @artsy/topaz-devs